support gRPC communication between primary and secondary mantle controllers #32

ushitora-anqou · 2024-08-26T04:05:59Z

No description provided.

satoru-takeuchi

I only reviewed the first commit so far. Please reflect my comments during my remaining reviews.

satoru-takeuchi · 2024-08-29T22:50:27Z

Makefile

@@ -52,7 +69,7 @@ manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and Cust
 	cat config/rbac/role.yaml | yq '.metadata.name = "mantle-controller"' > charts/mantle-cluster-wide/templates/clusterrole.yaml

 .PHONY: generate
-generate: controller-gen ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations.
+generate: $(PROTOBUF_GEN) controller-gen ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations.


Suggested change

generate: $(PROTOBUF_GEN) controller-gen ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations.

generate: $(PROTOBUF_GEN) controller-gen ## Generate boilerplate code.

Now generate target generates not only code for CR handling but also gRPC code.

satoru-takeuchi · 2024-08-29T22:57:37Z

cmd/controller/main.go

+		// nothing to do
+	case RolePrimary:
+		if mantleServiceEndpoint == "" {
+			return errors.New("--mantle-service-endpoint should be specified if --role is 'primary'")


Suggested change

return errors.New("--mantle-service-endpoint should be specified if --role is 'primary'")

return errors.New("--mantle-service-endpoint must be specified if --role is 'primary'")

satoru-takeuchi · 2024-08-29T22:57:47Z

cmd/controller/main.go

+		}
+	case RoleSecondary:
+		if mantleServiceEndpoint == "" {
+			return errors.New("--mantle-service-endpoint should be specified if --role is 'secondary'")


Suggested change

return errors.New("--mantle-service-endpoint should be specified if --role is 'secondary'")

return errors.New("--mantle-service-endpoint must be specified if --role is 'secondary'")

satoru-takeuchi · 2024-08-29T23:04:37Z

cmd/controller/main.go

+	managedCephClusterID := os.Getenv("POD_NAMESPACE")
+	if managedCephClusterID == "" {
+		setupLog.Error(errors.New("POD_NAMESPACE is empty"), "POD_NAMESPACE is empty")
+		return errors.New("POD_NAMESPACE is empty")
+	}


This code is not necessary since managedCephClusterId will be initialized in setupReconcilers.

satoru-takeuchi · 2024-08-29T23:12:20Z

cmd/controller/main.go

+
+	l, err := net.Listen("tcp", mantleServiceEndpoint)
+	if err != nil {
+		return fmt.Errorf("failed to listen: %w", err)


Suggested change

return fmt.Errorf("failed to listen: %w", err)

return fmt.Errorf("failed to listen %s: %w", mantleServiceEndpoint, err)

satoru-takeuchi · 2024-08-30T00:04:58Z

cmd/controller/main.go

+		return err
+	}
+
+	var wg sync.WaitGroup


Suggested change

var wg sync.WaitGroup

var wg sync.WaitGroup

wg.Add(1)

This is my mistake, and I should call wg.Add every time go is called. I'll fix my code so.

satoru-takeuchi · 2024-08-30T00:10:06Z

cmd/controller/main.go

+	}()
+
+	go func() {
+		defer wg.Done()


This line is unnecessary since wg.Done() will be called by L215. If this line exists, wg.Done() will be called twice and this will result in panic.

This is my mistake, and I should call wg.Add every time go is called. I'll fix my code so.

satoru-takeuchi · 2024-08-30T00:10:43Z

cmd/controller/main.go

+
+	go func() {
+		defer wg.Done()
+		_ = serv.Serve(l)


Checking error and logging is better.

satoru-takeuchi · 2024-08-30T00:33:58Z

internal/controller/mantlebackup_controller.go

@@ -20,13 +20,15 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

 	mantlev1 "github.com/cybozu-go/mantle/api/v1"
+	"github.com/cybozu-go/mantle/pkg/controller/proto"
 )

 // MantleBackupReconciler reconciles a MantleBackup object
 type MantleBackupReconciler struct {
 	client.Client
 	Scheme               *runtime.Scheme
 	managedCephClusterID string


How about adding role field? Although we can distinguish whether the role is primary by primarySettings, checking rolle == RolePrimary is more straightforward.

satoru-takeuchi · 2024-08-30T00:44:05Z

cmd/controller/main.go

+		<-ctx.Done()
+		serv.GracefulStop()
+	}()
+


Could you also call setupReconciler() for the secondary mantle and do nothing in Reconcile() if the rolle is secondary?

ushitora-anqou · 2024-08-30T04:03:45Z

I've fixed my code to resolve your points.

…ollers This commit allows a mantle-controller to serve as one of standalone, primary, or secondary mode. This commit doesn't have any e2e test's update and it will land on in the following commits. Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

All of the e2e tests we currently have are written for a single Kubernetes cluster. This commit moves them into a `singlek8s` package. Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

ushitora-anqou force-pushed the support-grpc-comm branch 7 times, most recently from 8575c8b to 6cbd772 Compare August 29, 2024 08:11

ushitora-anqou marked this pull request as ready for review August 29, 2024 08:47

ushitora-anqou requested a review from satoru-takeuchi August 29, 2024 08:47

satoru-takeuchi requested changes Aug 30, 2024

View reviewed changes

ushitora-anqou force-pushed the support-grpc-comm branch 2 times, most recently from ffe0e5a to 5e2a32a Compare August 30, 2024 02:49

ushitora-anqou added 3 commits August 30, 2024 05:38

move all e2e tests to singlek8s package

bd1f07e

All of the e2e tests we currently have are written for a single Kubernetes cluster. This commit moves them into a `singlek8s` package. Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

add simple e2e tests for replication

cdcc238

Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

ushitora-anqou force-pushed the support-grpc-comm branch from 5e2a32a to cdcc238 Compare August 30, 2024 05:38

ushitora-anqou requested a review from satoru-takeuchi August 30, 2024 06:50

satoru-takeuchi approved these changes Aug 30, 2024

View reviewed changes

satoru-takeuchi merged commit 6d00b20 into main Aug 30, 2024
3 checks passed

satoru-takeuchi deleted the support-grpc-comm branch August 30, 2024 08:21

satoru-takeuchi mentioned this pull request Sep 1, 2024

set proper status to MantleBackup #33

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support gRPC communication between primary and secondary mantle controllers #32

support gRPC communication between primary and secondary mantle controllers #32

ushitora-anqou commented Aug 26, 2024

satoru-takeuchi left a comment

satoru-takeuchi Aug 29, 2024

satoru-takeuchi Aug 29, 2024

satoru-takeuchi Aug 29, 2024

satoru-takeuchi Aug 29, 2024

satoru-takeuchi Aug 29, 2024

satoru-takeuchi Aug 30, 2024

ushitora-anqou Aug 30, 2024

satoru-takeuchi Aug 30, 2024

ushitora-anqou Aug 30, 2024 •

edited

Loading

satoru-takeuchi Aug 30, 2024

satoru-takeuchi Aug 30, 2024

satoru-takeuchi Aug 30, 2024

ushitora-anqou commented Aug 30, 2024 •

edited

Loading

	generate: $(PROTOBUF_GEN) controller-gen ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations.
	generate: $(PROTOBUF_GEN) controller-gen ## Generate boilerplate code.

	return errors.New("--mantle-service-endpoint should be specified if --role is 'primary'")
	return errors.New("--mantle-service-endpoint must be specified if --role is 'primary'")

	return fmt.Errorf("failed to listen: %w", err)
	return fmt.Errorf("failed to listen %s: %w", mantleServiceEndpoint, err)

support gRPC communication between primary and secondary mantle controllers #32

support gRPC communication between primary and secondary mantle controllers #32

Conversation

ushitora-anqou commented Aug 26, 2024

satoru-takeuchi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ushitora-anqou Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ushitora-anqou commented Aug 30, 2024 • edited Loading

ushitora-anqou Aug 30, 2024 •

edited

Loading

ushitora-anqou commented Aug 30, 2024 •

edited

Loading